Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional icon generator by generateIcons option #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jlsalvador
Copy link

The reason for this commit is that I use another custom generator for favicons (it generates PNGs from an SVG with a custom filename). Because of this, when esbuild-plugin-html tries to load the favicons referred URLs, it fails with the following error:

✘ [ERROR] Failed to resolve icon path: assets/icon-dark-196x196.png [plugin html]

✘ [ERROR] Failed to resolve icon path: assets/icon-dark-196x196.png [plugin html]

[build][resources][manifests]: 25.672ms
Error: Build failed with 2 errors:
error: Failed to resolve icon path: assets/icon-dark-196x196.png
error: Failed to resolve icon path: assets/icon-dark-196x196.png
    at failureErrorWithLog (node_modules/esbuild/lib/main.js:1472:15)
    at node_modules/esbuild/lib/main.js:945:25
    at node_modules/esbuild/lib/main.js:1353:9
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  errors: [Getter/Setter],
  warnings: [Getter/Setter]
}
[build]: 30.824ms
[build][resources][assets]: 38.637ms

Therefore, to ignore these errors, I added the build option generateIcons as a flag for the collectIcons.

The default value is true, so its behavior will be the same as always.

I added a test for generateIcons set to false.

Copy link

changeset-bot bot commented Jul 9, 2024

🦋 Changeset detected

Latest commit: bbc6195

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@chialab/esbuild-plugin-html Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jlsalvador
Copy link
Author

Hello,

Is there anything I can do to help you accept or review this PR?

Greetings.

jlsalvador added a commit to jlsalvador/rna that referenced this pull request Jul 25, 2024
This is required in order to use this library without compiling it after
a `npm ci`. Normally CICD create and publish npm dist packages, but
because I am waiting for a pull request on upstream [1], I publish here
my own changes while waiting for the upstream approvation.
[1] chialab#196
@edoardocavazza
Copy link
Member

Hello @jlsalvador! Sorry for the late reply. If you are still interested in pursuing this PR, could you please rebase and add the same option for the collectScreens method as well?

The reason for this commit is that I use another custom generator for
favicons (it generates PNGs from an SVG with a custom filename).
Because of this, when esbuild-plugin-html tries to load the favicons
referred URLs, it fails with the following error:

```
✘ [ERROR] Failed to resolve icon path: assets/icon-dark-196x196.png [plugin html]

✘ [ERROR] Failed to resolve icon path: assets/icon-dark-196x196.png [plugin html]

[build][resources][manifests]: 25.672ms
Error: Build failed with 2 errors:
error: Failed to resolve icon path: assets/icon-dark-196x196.png
error: Failed to resolve icon path: assets/icon-dark-196x196.png
    at failureErrorWithLog (node_modules/esbuild/lib/main.js:1472:15)
    at node_modules/esbuild/lib/main.js:945:25
    at node_modules/esbuild/lib/main.js:1353:9
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  errors: [Getter/Setter],
  warnings: [Getter/Setter]
}
[build]: 30.824ms
[build][resources][assets]: 38.637ms
```

Therefore, to ignore these errors, I added the build option
`generateFavicons` as a flag for the `collectIcons`.

The default value is `true`, so its behavior will be the same as
always.

I added a test for `generateFavicons` set to `false`.

Signed-off-by: José Luis Salvador Rufo <[email protected]>
@jlsalvador
Copy link
Author

Changes from V1 to V2:

  • Rebased from main.
  • Renamed generateIcons to generateFavicons.

Hello @jlsalvador! Sorry for the late reply. If you are still interested in pursuing this PR, could you please rebase and add the same option for the collectScreens method as well?

@edoardocavazza I think for collectScreens could be another option. For example, I might not want to generate the favicons but still need the Apple launcher icons. What do you think?

@edoardocavazza
Copy link
Member

I agree!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants